Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds unified settings with migration #9108

Merged
merged 8 commits into from
Jun 26, 2023

Conversation

adamint
Copy link
Member

@adamint adamint commented Jun 21, 2023

Fixes #8969

For some reason, the unified settings provider doesn't pick up UnifiedSettings folder if it is located in our MS.VS.Editors package. The current settings are located in that package, but in this PR are relocated to the ManagedProjectSystem package.

There is no projectsAndSolutions string, as one already exists (and we are warned about duplicates). I've manually confirmed this works.
image

Microsoft Reviewers: Open in CodeFlow

@adamint adamint requested a review from a team as a code owner June 21, 2023 20:51
@ghost ghost added the Feature-Tools-Options-UI "Tools | Options" pages in VS label Jun 21, 2023
@@ -657,3 +657,6 @@ folder "InstallDir:MSBuild\Microsoft\VisualStudio\Managed\zh-Hant"
file source="$(VisualStudioXamlRulesDir)zh-Hant\ResolvedProjectReference.xaml"
file source="$(VisualStudioXamlRulesDir)zh-Hant\ResolvedSdkReference.xaml"
file source="$(VisualStudioXamlRulesDir)zh-Hant\SdkReference.xaml"

folder "Extensions\Microsoft\ManagedProjectSystem\UnifiedSettings"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're required to place the settings in this specific place: ExtensionRoot\UnifiedSettings*.registration.json

@@ -657,3 +657,6 @@ folder "InstallDir:MSBuild\Microsoft\VisualStudio\Managed\zh-Hant"
file source="$(VisualStudioXamlRulesDir)zh-Hant\ResolvedProjectReference.xaml"
file source="$(VisualStudioXamlRulesDir)zh-Hant\ResolvedSdkReference.xaml"
file source="$(VisualStudioXamlRulesDir)zh-Hant\SdkReference.xaml"

folder "Extensions\Microsoft\ManagedProjectSystem\UnifiedSettings"
file source="$(VisualStudioExtensionSetupDir)\ProjectSystemSetup\UnifiedSettings\ManagedProjectSystem.registration.json"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we pick this up from output rather than the source project? The file has CopyToOutputDirectory so it would be copied somewhere.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just don't copy it to the output directory.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a static file, so it's not necessary to pick up from output. @tmeschter removed copying to output dir.

@drewnoakes
Copy link
Member

What's the developer experience like with unified settings? If you make a change to that JSON file and press F5, does it apply immediately?

Can we create a new .resx file for the strings used in settings?

@drewnoakes
Copy link
Member

What's the "migration" referred to in the PR title?

@drewnoakes
Copy link
Member

Would be good to confirm what's expected around capitalisation. My understanding is that we tend not to use All Caps In Titles Anymore. Instead we use sentence casing with only the first word in caps, unless its a proper noun. You can see this in our documentation on learn.microsoft.com, for example. We were careful to apply this throughout the new Project Properties UI, and a lot of other VS UI does the same, such as codefix labels and so forth.

The old UI capitalised all words, but that's a really old style UI. I don't think it suits the new UI.

"path": "ManagedProjectSystem\\FastUpToDateCheckEnabled"
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Migration" implies that the unified settings will move the current value of the setting as specified in the path and store it somewhere else going forward. Is that accurate? Any concerns about round-tripping the settings between VS installs using unified settings and those that don't? Or am I misunderstanding what this means?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No concerns about round-tripping, as this migration field takes care of converting legacy <-> unified settings location.
That's correct, they will be stored in a new location determined by the unified settings manager

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are settings stored in both locations then, at least for now? I wonder how conflicts are resolved.

"properties": {
"projectsAndSolutions.sdkStyleProjects.fastUpToDateCheck.enabled": {
"type": "boolean",
"title": "@Setting_FastUpToDateCheck_Enabled_Title;{860A27C0-B665-47F3-BC12-637E16A1050A}",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GUID identifies the VS package?

Copy link
Member Author

@adamint adamint Jun 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. I'd comment the json, if it were possible

@adamint
Copy link
Member Author

adamint commented Jun 22, 2023

What's the "migration" referred to in the PR title?

See #9108 (comment)

@adamint
Copy link
Member Author

adamint commented Jun 22, 2023

Would be good to confirm what's expected around capitalisation. My understanding is that we tend not to use All Caps In Titles Anymore. Instead we use sentence casing with only the first word in caps, unless its a proper noun. You can see this in our documentation on learn.microsoft.com, for example. We were careful to apply this throughout the new Project Properties UI, and a lot of other VS UI does the same, such as codefix labels and so forth.

The old UI capitalised all words, but that's a really old style UI. I don't think it suits the new UI.

Changed.

What's the developer experience like with unified settings? If you make a change to that JSON file and press F5, does it apply immediately?

No, and it currently has much room for improvement. I've been in touch with the Shell team, and they are working on a better developer experience, but we are one of the dogfooders here. You have to unzip the vsix and manually replace the installed package in the VS install dir. They do not pick up experimental instance extension data.

Can we create a new .resx file for the strings used in settings?

No, we cannot. I asked this question directly to the team. It must be in the VS package resx

@adamint adamint merged commit 44565c3 into dotnet:main Jun 26, 2023
@ghost ghost added this to the 17.7 milestone Jun 26, 2023
adamint pushed a commit to adamint/project-system that referenced this pull request Jun 27, 2023
adamint added a commit that referenced this pull request Jun 28, 2023
adamint pushed a commit to adamint/project-system that referenced this pull request Jul 5, 2023
adamint added a commit to adamint/project-system that referenced this pull request Jul 11, 2023
adamint added a commit that referenced this pull request Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature-Tools-Options-UI "Tools | Options" pages in VS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Tools | Options pages to use Unified Settings model
6 participants